-
Notifications
You must be signed in to change notification settings - Fork 0
Initial implementation #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
2700fe9 to
d40f200
Compare
d40f200 to
80a1cb0
Compare
|
Apologies for the noise, had the wrong email on my git config, so the CLA check failed. Fixed now. |
bkorycki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I haven't tried playing with it yet but the readme makes it seem simple to use which is awesome. I also think that in the future it would be very valuable to also be able to run non-local dvc datasets.
One thing I think we should maybe think about now is standardizing the experiment IDs. This would enable users to more easily dig through a large list of past runs. For example, the id could be automatically constructed from the sut ids/annotator ids/dataset name, and an optional tag. @bollacker may have more thoughts here.
Other than that I don't have any major notes! Looks like a solid first implementation to me.:)
src/modelplane/runways/responder.py
Outdated
| help="The number of jobs to run in parallel. Defaults to 1.", | ||
| ) | ||
| @load_from_dotenv | ||
| def get_responses( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit-pick but can you re-name the response/responder stuff to be something more specific to suts? Because "response" isn't a term unique to suts imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! Is there a term we use elsewhere that would make sense to re-use here? Or could just be get_sut_responses ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think get_sut_responses is good:)
@bkorycki , I like this idea, though I worry if we use all of those things to construct the experiment name, it'll be too long. One thing is that if we're good about the tagging, that should enable the nice searching. Though we're currently missing the sut_id tag from the annotator run; I'll add that. I'll add a note on this to the README under the TODOs! |
|
@bollacker merging so we can branch off of main for the next steps, but please still comment, if you like! |
Initial implementation for a workflow someone developing a new evaluator could use where experiment tracking is managed in mlflow.
I tried to explain what's happening in the README.md, and the best way to see it is via the template jupyter notebook.
Opening as draft for feedback!